Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds notes about HOC to handle client routing #3168

Closed
wants to merge 7 commits into from
Closed

adds notes about HOC to handle client routing #3168

wants to merge 7 commits into from

Conversation

gregorskii
Copy link

@gregorskii gregorskii commented Dec 8, 2017

Hi all,

This is per the convo on Discord this morning about a HOC for client URL params.

I am having some issues testing this in build mode... wanted to mention it here.

When using this config a new page is created in the public output as /public/widgets/view/index.html but the the route does not work in the browser. Going to /widgets/view/10 is a 404, widgets/view/index.html works but does not allow one to include the ID.

This feels like something NGINX configs would solve, but it is supposed to work on S3 as well. S3 might work with an additional page setup as a redirect. What would be expected here?

Thanks

@gatsbybot
Copy link
Collaborator

gatsbybot commented Dec 8, 2017

Deploy preview ready!

Built with commit d55e34a

https://deploy-preview-3168--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Dec 8, 2017

Deploy preview ready!

Built with commit 31d476a

https://deploy-preview-3168--using-drupal.netlify.com

@pieh
Copy link
Contributor

pieh commented Dec 9, 2017

It's kind of expected that entering parameterized url without any server configuration will return 404. But if you navigate to /widgets/view/10 using gatsby-link it will work in production build too as this will use react-router and not server routing. Perhaps worth adding note that there will be needed additional server configuration when using this feature.

If parameterized url would be "popular" then maybe it would be worth creating plugin to automaticly generate config/redirect files for servers (nginx / apache) / services (netlify / gh-pages / surge.sh). That is if that's actually something possible in those services - didn't really do research about that.

Great job 👍

@gregorskii
Copy link
Author

Awesome. Will try using gatsby link to get to the pages, the particular way these links were made required me to push history.

Either way a reload of the page will result in a 404, so a note about server config is likely needed.

This app I am setting up is using nginx, I’ll try creating the redirects and see what happens. If it was hosted on S3 it will either need redirects or not work at all? :/

I’ll see what I come up with this weekend.

Thanks!

@gregorskii
Copy link
Author

This is ready, added notes about server config, and got it working for my use case in NGINX.

@KyleAMathews
Copy link
Contributor

If parameterized url would be "popular" then maybe it would be worth creating plugin to automatically generate config/redirect files for servers (nginx / apache) / services (netlify / gh-pages / surge.sh)

Yes! This is exactly what I've hoped would happen. The only service with a plugin afaik right now is Netlify — which actually isn't yet handling creating server config for client-only routes. Would love to see similar plugins be created for all the others.

@KyleAMathews
Copy link
Contributor

@gregorskii thanks for this PR! Looks great.

@ghost ghost assigned KyleAMathews Dec 13, 2017
@ghost ghost added the review label Dec 13, 2017

**Client:**

Extracting path params from the route on the client requires that you use the `react-router` `<Route>` in your component.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised to hear this — the param information should be available without any more work on props.location — is this not so?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I was under the impression from convos in discord that you still needed to create client routes for the page. I could not access the route Params without it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you verify what the props.location looks like w/o the custom client routes? If it's not working, we should fix that in Gatsby's auto-created route components as it's a lot of overhead to make people add their own route components.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check tomorrow, fairly certain it was empty without the route.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Params are not passed to components.

I searched repository for matchPath and it seems gatsby only tests if path is matched and discard return value of react-router matchPath (which would have extracted params):

if (page.matchPath) {
// Try both the path and matchPath
if (
matchPath(trimmedPathname, { path: page.path }) ||
matchPath(trimmedPathname, {
path: page.matchPath,
})
) {
foundPage = page
pageCache[trimmedPathname] = page
return true
}
} else {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the research @pieh — so yeah, we need to fix the problem there — pass into the component the props for the matched path. @gregorskii want to tackle this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I will look into it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregorskii btw, do you think you'll still have a chance to look into this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KyleAMathews I have not had a chance to look into it. I got pulled into another project with a short timeline at work.

I can try to look at it after, but if its a quick fix, does someone else have time?

I was able to solve this on my project with the proposed fix in this original PR, however as discussed my work around may not be needed.

@KyleAMathews
Copy link
Contributor

Hey, re-reading this I'm not sure if this is worth adding to the creating pages docs page. It's a nice pattern but not a huge improvement over just writing routes directly and it's something most intermediate+ React users would figure out themselves.

I think this would make a nice blog post or if we ever add a tips & tricks type section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants